Skip to content

Fixed #7041 - Toggle size effect demo not working.#203

Closed
kzys wants to merge 5 commits intojquery:masterfrom
kzys:trac-7041
Closed

Fixed #7041 - Toggle size effect demo not working.#203
kzys wants to merge 5 commits intojquery:masterfrom
kzys:trac-7041

Conversation

@kzys
Copy link
Contributor

@kzys kzys commented May 5, 2011

Size effect's problems are

  • Don't swap from/to on "show" mode.
  • Modify some properties with setTransition, but don't save/restore these properties.

In this branch, I've fixed these problems to close #7041.

…more properties. Fixed #7041 - Toggle size effect demo not working.
@gnarf
Copy link
Member

gnarf commented May 5, 2011

Also, you might wanna make the changes from this commit to see what happens in another "toggle" scenario.

 * Separete `scale` effect's problem.
 * Clearify the default value of `from` and `to`.
 * Follow http://docs.jquery.com/JQuery_Core_Style_Guidelines.
@kzys
Copy link
Contributor Author

kzys commented May 5, 2011

Thank you Corey. I've added new commit to clean up my code. Please review again.

@kzys
Copy link
Contributor Author

kzys commented May 5, 2011

Thank you! I've merged your suggestion.

@gnarf
Copy link
Member

gnarf commented May 5, 2011

@kzys sorry if I didn't explain myself fully, that first half of the if was still totally valid, that was just a replacement for the else case... Also, could you maybe move that zero var definition up into the var statement?

@kzys
Copy link
Contributor Author

kzys commented May 5, 2011

Finally I understand your point.

On #sizeToggle of "visual effects test" page (It's very helpful. Thank you), First toggle hide element and second toggle show element again. And second animation looks odd because it's zero -> original.

However, It's problem of show, not toggle. We should swap from/to on show because other effect do this. For example, $(shown).hide('slide', { direction: 'left' }); is slide to left, but $(hidden).show('slide', { direction: 'left' }); is slide from left.

@gnarf
Copy link
Member

gnarf commented May 7, 2011

I think I'm sold on the reverse direction for show... All the time... Because of your point about the directions on slide/etc. We really need to document that behavior though...

@gnarf
Copy link
Member

gnarf commented May 10, 2011

For right now, can you put the if back in that works for toggle show only? I'd like to at least get this change in to fix this bug for the toggle case... We can further discuss the fate of the rest of show animations for size later...

@kzys
Copy link
Contributor Author

kzys commented May 12, 2011

Like this?

@gnarf
Copy link
Member

gnarf commented May 12, 2011

Not quite, you still need the direction detection in the "else" :

if ( o.mode === "toggle" && mode === "show" ) {
    el.from = o.to || zero;
    el.to = o.from || original;
} else {
    el.from = o.from || ( mode === "show" ? zero : original );
    el.to = o.to || ( mode === "hide" ? zero : original );
}

@kzys
Copy link
Contributor Author

kzys commented May 12, 2011

Oh, Sorry. I've fixed (actually just a copy-and-paste...).

@gnarf
Copy link
Member

gnarf commented Jun 10, 2011

This no longer wants to merge cleanly, could you rebase / merge with changes upstream?

Also, make sure to cover the whitespace issues :)

@gnarf
Copy link
Member

gnarf commented Jun 21, 2011

There have been a lot of changes upstream, I'd like to keep this fix, if you want to re-make it on top of a clean master that would be awesome. Please re-open / make a new pull when you get this mergable.

@gnarf gnarf closed this Jun 21, 2011
@kzys
Copy link
Contributor Author

kzys commented Jun 22, 2011

Okay.

gnarf pushed a commit that referenced this pull request Oct 24, 2012
kborchers pushed a commit that referenced this pull request Nov 1, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants